feat: fusdc status manager states #10406

0xpatrickdev · 2024-11-05T22:33:50Z

closes: #10389

Description

StatusManager with OBSERVED, ADVANCED states tracked in local MapStore
StatusManager state updates via Settler and Advancer
CctpTxEvidence type, typeGuard, and fixtures

Security Considerations

See FastUSDC threat model

Scaling Considerations

includes a seenTxs SetStore that will keep track of every tx observed by fusdc contract to assert uniqueness

Documentation Considerations

Includes state diagram in exos/README.md and the usual jsdoc

Testing Considerations

Includes unit tests of exos with a mocked LCA.

Upgrade Considerations

None, unreleased

cloudflare-workers-and-pages · 2024-11-05T22:34:53Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`f3d1e36`
Status:	✅ Deploy successful!
Preview URL:	https://1f60b94a.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-fusdc-status-manager.agoric-sdk.pages.dev

View logs

packages/fast-usdc/src/constants.js

packages/fast-usdc/src/exos/README.md

dckc · 2024-11-06T03:00:15Z

packages/fast-usdc/src/exos/settler.js

+          // TODO FAILURE PATH -> put money in recovery account or .transfer to receiver
+          // TODO should we have a TxStatus for this?
+          throw makeError(
+            `🚨 No pending settlement found for ${q(tx.sender)} ${q(tx.amount)}`,


Is this recoverable? It looks like it.

Suggested change

`🚨 No pending settlement found for ${q(tx.sender)} ${q(tx.amount)}`,

`⚠️ No pending settlement found for ${q(tx.sender)} ${q(tx.amount)}`,

per Logging docs:

🚨 indicates something that should never happen.

⚠️ indicates something that is not expected but was recovered from.

🚨 was intentional - I think we should leave it until we recover from here

We might want to capture this in the StatusManager with an ORPHANED state, and add some logic in the observe() method to initiate a SETTLE / settlement

Let's come back to this when we're going all the error cases

packages/fast-usdc/src/exos/status-manager.js

dckc · 2024-11-06T03:22:22Z

packages/fast-usdc/src/exos/status-manager.js

+          ...mutableEntries[unsettledIdx],
+          status: TxStatus.SETTLED,
+        };
+        txs.set(key, harden(mutableEntries));


Do we ever delete from txs?

Updated the approach to remove entries once they get to Settled

dckc · 2024-11-06T04:19:47Z

packages/fast-usdc/src/exos/status-manager.js

+       * @param {bigint} amount
+       */
+      settle(address, amount) {
+        const key = `${address}-${amount}`;


The exo guard doesn't guarantee that address includes no - characters. This sets off my confusion-attack spidey-sense.

Consider...

Suggested change

const key = `${address}-${amount}`;

const key = JSON.stringify([address, `${amount}`]);

Please see this fixup: 5226323

Another alternative: put the number first

Or split from the right? Hm

There are no reqs currently to decompose the key, so I think the current approach is sound. If anything, JSON.stringify might be overkill vs simple concatenation.

This is now factored to its own function and easy to change in the future.

There are no reqs currently to decompose the key

Good point. It could even be a hash function. Let's leave it as is for debug-ability and we can button up before release

turadg

Good stuff! Most of my feedback I'd be content to see a TODO so that we can land this and keep iterating.

packages/fast-usdc/src/constants.js

packages/fast-usdc/src/exos/README.md

packages/fast-usdc/src/exos/advancer.js

turadg · 2024-11-06T22:53:04Z

packages/fast-usdc/src/exos/advancer.js

+       */
+      handleEvent(event) {
+        // observe regardless of validation checks
+        // TODO - should we only observe after validation checks?


yeah it's not a transaction unless it's valid.

but validation checks should be performed by the Feed exo so it only forwards what is legitimate. The advancer needn't be concerned with query params.

There's no Feed yet so this could have a TODO but please make the plan clear. Perhaps add it to the issue about making the feed

packages/fast-usdc/src/exos/status-manager.js

turadg · 2024-11-06T23:01:36Z

packages/fast-usdc/src/exos/status-manager.js

+          return 0;
+        }
+        const current = txs.get(key);
+        // XXX do we need defensive logic to ensure multiple entries don't have


a fine invariant to assert. it's recoverable by ignoring duplicates. we don't put that burden on the feed because it would have to keep a record of all transactions. this storage can do the job

Ok I'll save that for a later PR, but we can use txHash + chainId to assert uniqueness.

Thinking we'd want a new SetStore to keep track of these, since the txs MapStore ejects entries once they're Settled?

Implemented in 6883b74

turadg · 2024-11-06T23:03:37Z

packages/fast-usdc/src/exos/status-manager.js

+        txs.set(key, harden(mutableEntries));
+      },
+      /**
+       * @overload


TIL! you can also do this in a TS expression inside a @type comment. give that a try because it would be harder to miss. (My eyes only looked up to the lowest /** and didn't think to keep searching)

Ok iIll try that. This was a pain to wrangle and I wasn't happy with the current outcome

No luck with this:

/** * @type {((address: NobleAddress, amount: bigint) => StatusManagerValue[]) | ((address: NobleAddress, amount: bigint, index: number) => StatusManagerValue | undefined)} */

Was also playing around with these:

/** * @type {<T extends number | undefined = undefined>( * address: NobleAddress, * amount: bigint, * index?: T * ) => T extends number ? StatusManagerValue | undefined : StatusManagerValue[]} */

/** * @template {number | undefined} [T=undefined] * @param {NobleAddress} address * @param {bigint} amount * @param {T} [index] * @returns {T extends number ? StatusManagerValue | undefined : StatusManagerValue[]} */

~~I also don't know if the index logic in StatusManager is here to say. Some of the convos in other threads lead me to believe we won't need it. So not going to spin wheels too hard on this rn.~~

Edit: the index logic did not stay around, so this is irrelevant

packages/fast-usdc/src/typeGuards.js

turadg · 2024-11-06T23:07:13Z

packages/fast-usdc/src/utils/address.js

+import { makeError, q } from '@endo/errors';
+
+/**
+ * Very minimal 'URL query string'-like parser that handles:


do we not have access to URL in XS?

I'm not sure - but agoric123?foo=bar is not accepted in a URL constructor in a web console.

qs, query-string, and querystringify are well-known libraries we might consider using if this were to be provided by @agoric/orchestration. This particular contract seems to have much simpler reqs.

No, we don't have URL in XS, I'm pretty sure.

Glad to see the tests.

qs-lite.min.js in https://github.com/kawanet/qs-lite is 530 bytes. Maybe easier to source that to get a more conventional qs ability

Cool, xs seems to have decodeURIComponent too: https://github.com/Moddable-OpenSource/moddable/blob/978945595c6aaa460c6068b34d3ebc415db248a7/xs/sources/xsCommon.c#L1375

I suppose we can get by without decodeURIComponent for now but consider mentioning in the code that it's available in XS

turadg · 2024-11-08T22:35:34Z

packages/fast-usdc/src/constants.js

+ *
+ * @enum {(typeof WriterTxStatus)[keyof typeof WriterTxStatus]}
+ */
+export const WriterTxStatus = /** @type {const} */ ({


helpful distinction. please make the unqualified name be this all-encompassing enum. Observed and Advanced are the subset of a transaction status that are recorded in local state.

turadg · 2024-11-08T22:36:08Z

packages/fast-usdc/src/exos/README.md

+  Settled --> [*]
+```
+
+### Complete state diagram (starting from OCW)


turadg · 2024-11-08T22:37:59Z

packages/fast-usdc/src/exos/advancer.js

+        /** @type {ChainAddress} */
+        const destination = harden({
+          chainId,
+          value: EUD,
+          encoding: /** @type {const} */ ('bech32'),
+        });


probably worth a helper fn

Agree, what would we call it? toChainAddress might not capture the bech32 we're adding. toBech32ChainAddress seemed too verbose where it wasn't worth factoring out.

Maybe ChainHub's getChainInfoByAddress should do this and instead be getChainAddress(addr)?

+1 to putting it on ChainHub since chainId is used here only for this construction.

We shouldn't reuse getChainInfoByAddress because this is making a value, not getting one.

consider,

Suggested change

/** @type {ChainAddress} */

const destination = harden({

chainId,

value: EUD,

encoding: /** @type {const} */ ('bech32'),

});

const destination = chainHub.makeChainAddress(EUD);

Ok, please see 990a6df. I decided not to label this a breaking change since getChainInfoByAddress has only been around for ~a week, but let me know if I should amend the commit message.

turadg · 2024-11-08T22:42:06Z

packages/fast-usdc/src/exos/status-manager.js

+      observe: M.call(CctpTxEvidenceShape).returns(M.undefined()),
+      hasPendingSettlement: M.call(M.string(), M.bigint()).returns(M.boolean()),
+      settle: M.call(M.string(), M.bigint()).returns(M.undefined()),
+      view: M.call(M.string(), M.bigint()).returns(M.arrayOf(PendingTxShape)),


this is a new name for what is usually called lookup. It also reads as if it could be a state transition. Please change.

turadg · 2024-11-08T22:45:58Z

packages/fast-usdc/src/exos/status-manager.js

+        assertNotSeen(evidence);
+        const key = getPendingTxKey(evidence);
+        const entry = { ...evidence, status: TxStatus.Observed };
+
+        appendToStoredArray(
+          // @ts-expect-error index signature for type 'string' is missing in type 'PendingTx'.
+          pendingTxs,
+          key,
+          entry,
+        );


please DRY the duplication with advance using a helper in the prepare scope. e.g.

Suggested change

assertNotSeen(evidence);

const key = getPendingTxKey(evidence);

const entry = { ...evidence, status: TxStatus.Observed };

appendToStoredArray(

// @ts-expect-error index signature for type 'string' is missing in type 'PendingTx'.

pendingTxs,

key,

entry,

);

addTransaction(evidence, TxStatus.Observed);

turadg · 2024-11-08T22:53:43Z

packages/fast-usdc/src/utils/address.js

+import { makeError, q } from '@endo/errors';
+
+/**
+ * Very minimal 'URL query string'-like parser that handles:


Glad to see the tests.

qs-lite.min.js in https://github.com/kawanet/qs-lite is 530 bytes. Maybe easier to source that to get a more conventional qs ability

turadg · 2024-11-11T17:55:32Z

packages/fast-usdc/src/exos/advancer.js

+       * @param {CctpTxEvidence} evidence
+       */
+      handleTransactionEvent(evidence) {
+        // XXX assume EventFeed performs validation checks. Advancer will assert uniqueness.


not tech debt, this is part of the design

turadg · 2024-11-11T17:55:56Z

packages/fast-usdc/src/exos/advancer.js

+    config => harden(config),
+    {
+      /**
+       * XXX For now, assume this is invoked when a new entry is


why make this assumption? The advancer should not be concerned

turadg · 2024-11-11T17:56:44Z

packages/fast-usdc/src/exos/advancer.js

+          requestedAmount,
+        );
+
+        // mark as Advanced since we're initiating the advance


already started

Suggested change

// mark as Advanced since we're initiating the advance

// mark as Advanced since `.transfer` initiates the advance

turadg · 2024-11-11T18:00:30Z

packages/fast-usdc/src/types.ts

+/** composite of NobleAddress and transaction amount value */
+export type PendingTxKey = `"["${NobleAddress}",${bigint}]"`;


should not be parsable.

Suggested change

/** composite of NobleAddress and transaction amount value */

export type PendingTxKey = `"["${NobleAddress}",${bigint}]"`;

/** composite of NobleAddress and transaction amount value */

export type PendingTxKey = `"pendingTx:${string}`;

turadg · 2024-11-11T18:01:02Z

packages/fast-usdc/src/exos/status-manager.js

+ * @param {bigint} amount
+ */
+const toPendingTxKey = (addr, amount) =>
+  /** @type {PendingTxKey} */ (JSON.stringify([addr, String(amount)]));


please make clear in the docs that this isn't meant to be parseable. see also PendingTxKey type comment

turadg · 2024-11-11T18:13:34Z

packages/fast-usdc/src/utils/address.js

+import { makeError, q } from '@endo/errors';
+
+/**
+ * Very minimal 'URL query string'-like parser that handles:


I suppose we can get by without decodeURIComponent for now but consider mentioning in the code that it's available in XS

turadg · 2024-11-11T18:17:24Z

packages/fast-usdc/src/utils/address.js

+   * @returns {boolean}
+   */
+  hasQueryParams: address => {
+    return address.includes('?');


agoric1foo?bar?baz would return true but is invalid. for this to be true it getQueryParams should not error.

Suggested change

return address.includes('?');

try {

addressTools.getQueryParams(address);

return result.params.length > 0;

} catch {

return false;

}

turadg · 2024-11-11T18:23:32Z

packages/fast-usdc/test/constants.test.ts

+  const txStatuses = values(TxStatus);
+  const invalidStatuses = values(PendingTxStatus).find(
+    status => !txStatuses.includes(status),
+  );
+  t.is(invalidStatuses, undefined, `${invalidStatuses} not in TxStatus`);


I was thrown off by the comparison with undefined.

Suggested change

const txStatuses = values(TxStatus);

const invalidStatuses = values(PendingTxStatus).find(

status => !txStatuses.includes(status),

);

t.is(invalidStatuses, undefined, `${invalidStatuses} not in TxStatus`);

const allStatusus = new Set(values(TxStatus));

const pendingStatuses = new Set(values(PendingTxStatus));

const pendingMinusAll = pendingStatuses.difference(allStatuses);

t.is(pendingMinusAll.size, 0);

The main intention here was to ensure the extraneous value name(s) are in the error message. TIL about Set.difference, pretty cool. Unfortunately in our testing env, I'm seeing pendingStatuses.difference is not a function. I went with a slightly revised approach that's hopefully more readable: 9c14032

Oh, it's not available until Node 22. When we can use it, isSubsetOf would be even better. But the filter is fine.

packages/fast-usdc/test/utils/address.test.ts

packages/orchestration/src/exos/chain-hub.js

turadg · 2024-11-12T19:12:15Z

packages/fast-usdc/src/exos/status-manager.js

+      },
+
+      /**
+       * Lookup all entries for a given address and amount


all the pending entries. maybe also lookupPending in the name

turadg · 2024-11-12T19:12:53Z

packages/fast-usdc/src/types.ts

+
+export type LogFn = (...args: unknown[]) => void;
+
+export interface PendingTx extends CctpTxEvidence {


we should do more of this, interfaces extending interfaces

- removes `getChainInfoByAddress` and provides `makeChainAddress`, better aligning the helper with intended usage - arguably a breaking change, but we've yet to release `getChainInfoByAddress`

- shows the various `StatusManager` MapStore states updates (`OBSERVED`, `ADVANCED`) via `Advancer` and `Settler` stubs

- use a composite key of `txHash+chainId` to track unique `EventFeed` submissions

0xpatrickdev force-pushed the pc/fusdc-status-manager branch from af07f34 to a4ae691 Compare November 6, 2024 00:32

turadg reviewed Nov 6, 2024

View reviewed changes

packages/fast-usdc/src/constants.js Outdated Show resolved Hide resolved

packages/fast-usdc/src/exos/README.md Outdated Show resolved Hide resolved

dckc reviewed Nov 6, 2024

View reviewed changes

packages/fast-usdc/src/exos/status-manager.js Outdated Show resolved Hide resolved

dckc reviewed Nov 6, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/fusdc-status-manager branch 5 times, most recently from 4cb1978 to 5226323 Compare November 6, 2024 22:19

0xpatrickdev marked this pull request as ready for review November 6, 2024 22:21

0xpatrickdev requested a review from a team as a code owner November 6, 2024 22:21

0xpatrickdev requested review from thams, dckc, turadg and samsiegart November 6, 2024 22:21

turadg reviewed Nov 6, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/fusdc-status-manager branch 6 times, most recently from 444aedd to 88f51f3 Compare November 8, 2024 15:14

0xpatrickdev requested a review from turadg November 8, 2024 15:15

turadg reviewed Nov 8, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/fusdc-status-manager branch 3 times, most recently from aa7d0dd to 6883b74 Compare November 11, 2024 16:41

turadg reviewed Nov 11, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/fusdc-status-manager branch 2 times, most recently from 8acd20c to a92b0ec Compare November 11, 2024 19:54

turadg mentioned this pull request Nov 12, 2024

10387 simple transaction feed for Fast USDC #10452

Merged

turadg approved these changes Nov 12, 2024

View reviewed changes

0xpatrickdev added 7 commits November 12, 2024 14:21

feat: TxStatus const for StatusManager states

1376020

types: CctpTxEvidence, PendingTx

646df78

test: MockCctpTxEvidences and MockVTransferEvents fixtures

5e4139c

feat: CctpTxEvidenceShape, PendingTxShape typeGuards

5a7b3d2

chore: export denomHash helper

1063efd

feat: minimal addressTools for query param parsing

6f97e13

refactor: getChainInfoByAddress -> makeChainAddress

3f71342

- removes `getChainInfoByAddress` and provides `makeChainAddress`, better aligning the helper with intended usage - arguably a breaking change, but we've yet to release `getChainInfoByAddress`

0xpatrickdev force-pushed the pc/fusdc-status-manager branch from a92b0ec to 9eac623 Compare November 12, 2024 19:21

0xpatrickdev added automerge:rebase Automatically rebase updates, then merge and removed automerge:rebase Automatically rebase updates, then merge labels Nov 12, 2024

0xpatrickdev added 2 commits November 12, 2024 14:26

feat: StatusManager scaffold

980463f

- shows the various `StatusManager` MapStore states updates (`OBSERVED`, `ADVANCED`) via `Advancer` and `Settler` stubs

feat: StatusManager tracks seenTxs

f3d1e36

- use a composite key of `txHash+chainId` to track unique `EventFeed` submissions

0xpatrickdev force-pushed the pc/fusdc-status-manager branch from 9eac623 to f3d1e36 Compare November 12, 2024 19:27

0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Nov 12, 2024

mergify bot merged commit c236c55 into master Nov 12, 2024
91 checks passed

mergify bot deleted the pc/fusdc-status-manager branch November 12, 2024 20:25

	`🚨 No pending settlement found for ${q(tx.sender)} ${q(tx.amount)}`,
	`⚠️ No pending settlement found for ${q(tx.sender)} ${q(tx.amount)}`,

	const key = `${address}-${amount}`;
	const key = JSON.stringify([address, `${amount}`]);

	// mark as Advanced since we're initiating the advance
	// mark as Advanced since `.transfer` initiates the advance

		/** composite of NobleAddress and transaction amount value */
		export type PendingTxKey = `"["${NobleAddress}",${bigint}]"`;

-    return address.includes('?');
+    try {
+      addressTools.getQueryParams(address);
+      return result.params.length > 0;
+    } catch {
+      return false;
+     }


		export type LogFn = (...args: unknown[]) => void;

		export interface PendingTx extends CctpTxEvidence {

feat: fusdc status manager states #10406

feat: fusdc status manager states #10406

Conversation

0xpatrickdev commented Nov 5, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Nov 5, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

0xpatrickdev Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev commented Nov 5, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Nov 5, 2024 •

edited

Loading

0xpatrickdev Nov 6, 2024 •

edited

Loading

turadg Nov 8, 2024 •

edited

Loading

0xpatrickdev Nov 7, 2024 •

edited

Loading

0xpatrickdev Nov 7, 2024 •

edited

Loading

turadg Nov 12, 2024 •

edited

Loading